Skip to content

fix: use in-memory cache in price service#427

Merged
aganglada merged 5 commits into
mainfrom
fix/prices-with-inmemory-cache
Jun 23, 2025
Merged

fix: use in-memory cache in price service#427
aganglada merged 5 commits into
mainfrom
fix/prices-with-inmemory-cache

Conversation

@aganglada

@aganglada aganglada commented Jun 23, 2025

Copy link
Copy Markdown
Contributor
  • Swap StateCache to InMemoryCache in PriceApiClient
  • Implement TTL logic for the InMemoryCache

@aganglada aganglada self-assigned this Jun 23, 2025
xavier-brochard
xavier-brochard reviewed Jun 23, 2025
View reviewed changes
Comment thread packages/snap/src/core/caching/InMemoryCache.ts Outdated
xavier-brochard
xavier-brochard reviewed Jun 23, 2025
View reviewed changes
Comment thread packages/snap/src/core/caching/InMemoryCache.ts Outdated
cursor[bot]

This comment was marked as outdated.

Sign in to view

FrederikBolding
FrederikBolding reviewed Jun 23, 2025
View reviewed changes
Comment thread packages/snap/src/snapContext.ts
cursor[bot]

This comment was marked as outdated.

Sign in to view

xavier-brochard
xavier-brochard reviewed Jun 23, 2025
View reviewed changes
Comment thread packages/snap/src/core/caching/InMemoryCache.ts Outdated
cursor[bot]

This comment was marked as outdated.

Sign in to view

cursor[bot]
cursor Bot reviewed Jun 23, 2025
View reviewed changes

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Cache Inconsistency with Expired Entries

The InMemoryCache's mget method inconsistently handles expired entries. While it calls #cleanupExpiredEntries() at the beginning, it does not check individual entries for expiration before returning them, unlike peek() and has(). This can lead to returning expired values if an entry expires between the initial cleanup and individual access, and results in incorrect "cache hit" logs for expired entries.

packages/snap/src/core/caching/InMemoryCache.ts#L122-L143

snap-solana-wallet/packages/snap/src/core/caching/InMemoryCache.ts

Lines 122 to 143 in 8dc73d7

async mget(
keys: string[],
): Promise<Record<string, Serializable | undefined>> {
await this.#cleanupExpiredEntries();
const result: Record<string, Serializable | undefined> = {};
for (const key of keys) {
const cacheEntry = this.#cache.get(key);
if (!cacheEntry) {
this.logger.info(`[InMemoryCache] ❌ Cache miss for key "${key}"`);
result[key] = undefined;
continue;
}
this.logger.info(`[InMemoryCache] 🎉 Cache hit for key "${key}"`);
result[key] = cacheEntry.value;
}
return result;
}

Fix in Cursor

Was this report helpful? Give feedback by reacting with 👍 or 👎

@sonarqubecloud

sonarqubecloud Bot commented Jun 23, 2025

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
9.9% Duplication on New Code (required ≤ 7%)

See analysis details on SonarQube Cloud

@aganglada
aganglada merged commit 7855ec9 into main Jun 23, 2025
9 of 10 checks passed
@aganglada
aganglada deleted the fix/prices-with-inmemory-cache branch June 23, 2025 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FrederikBolding FrederikBolding FrederikBolding left review comments
@cursor cursor[bot] cursor[bot] left review comments
@xavier-brochard xavier-brochard xavier-brochard approved these changes

Assignees

@aganglada aganglada

Labels

area-performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aganglada @FrederikBolding @xavier-brochard @dylanbutler1